fix: amm-1927 send headers only if the request is from the allowed origin #110
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis PR introduces comprehensive CORS handling enhancements across the request processing pipeline. It adds PATCH method support and expands allowed headers in global CORS configuration, implements preflight (OPTIONS) request handling with origin validation in the JWT filter, and adds per-origin CORS header logic to the HTTP interceptor. Changes
Sequence DiagramsequenceDiagram
actor Client
participant Filter as JwtUserIdValidationFilter
participant Interceptor as HTTPRequestInterceptor
participant App as Application
rect rgb(200, 230, 255)
Note over Client,App: CORS Preflight (OPTIONS)
Client->>Filter: OPTIONS request + Origin header
Filter->>Filter: Capture Origin, Method, URI
Filter->>Filter: Validate Origin against allowed list
alt Origin allowed
Filter->>Client: 200 OK + CORS headers
else Origin not allowed
Filter->>Client: 403 Forbidden
end
end
rect rgb(200, 255, 220)
Note over Client,App: Actual Request
Client->>Filter: GET/POST/PUT/PATCH/DELETE + Origin
Filter->>Filter: Validate Origin if present
alt Origin allowed or absent
Filter->>Interceptor: Continue processing
Interceptor->>Interceptor: Check allowed origin
Interceptor->>App: Forward request
App->>Interceptor: Response
alt Error response & Origin allowed
Interceptor->>Interceptor: Add CORS headers + Credentials
end
Interceptor->>Client: Response + CORS headers (if origin valid)
else Origin rejected
Filter->>Client: 403 Forbidden
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/com/iemr/admin/utils/JwtUserIdValidationFilter.java (1)
160-164: Critical: Duplicate error handling code.Lines 160-161 are duplicated at lines 163-164. This will log the warning message twice and attempt to send the error response twice, though the second call will likely fail since the response is already committed after the first
sendError.Remove the duplicate lines:
logger.warn("No valid authentication token found"); response.sendError(HttpServletResponse.SC_UNAUTHORIZED, "Unauthorized: Invalid or missing token"); - - logger.warn("No valid authentication token found"); - response.sendError(HttpServletResponse.SC_UNAUTHORIZED, "Unauthorized: Invalid or missing token");
♻️ Duplicate comments (1)
src/main/java/com/iemr/admin/utils/JwtUserIdValidationFilter.java (1)
172-188: Code duplication and localhost pattern change.This method duplicates identical logic in HTTPRequestInterceptor (lines 153-166) - see the comment on that file for a refactoring suggestion to eliminate duplication.
Additionally, line 183 simplified the wildcard regex handling by removing special localhost port handling that may have existed previously. Verify that local development configurations still work correctly with this change, especially if developers use different ports.
Run the following script to check if there are any localhost-specific patterns in configuration files that might be affected:
#!/bin/bash # Description: Check for localhost patterns in configuration files # Search for localhost patterns in application properties fd -e properties -e yml -e yaml | xargs rg -n "localhost" -A 2 -B 2 # Search for CORS configuration fd -e properties -e yml -e yaml | xargs rg -n "cors.allowed-origins" -A 2 -B 2
🧹 Nitpick comments (1)
src/main/java/com/iemr/admin/config/CorsConfig.java (1)
23-24: Explicit header allowlist improves security.Replacing the wildcard "*" with an explicit list of allowed headers is a security improvement. However, the inclusion of four case variations of "serverAuthorization" suggests potential client-side inconsistencies.
Consider normalizing header handling on the client side to eliminate the need for multiple case variations, which would simplify maintenance.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/main/java/com/iemr/admin/config/CorsConfig.java(1 hunks)src/main/java/com/iemr/admin/utils/JwtUserIdValidationFilter.java(3 hunks)src/main/java/com/iemr/admin/utils/http/HTTPRequestInterceptor.java(4 hunks)
🔇 Additional comments (5)
src/main/java/com/iemr/admin/config/CorsConfig.java (1)
22-22: LGTM: PATCH method support added.Adding PATCH to the allowed methods list completes REST API support and aligns with the stated PR objectives.
src/main/java/com/iemr/admin/utils/http/HTTPRequestInterceptor.java (2)
47-48: LGTM: Origin configuration properly injected.The allowedOrigins field correctly reuses the same configuration property as CorsConfig, ensuring consistency across the application.
112-118: Origin validation added for error responses.The origin-aware CORS header logic correctly validates origins before adding headers to error responses. Note that this interceptor only handles error responses, while successful responses rely on Spring's CorsConfig for CORS handling.
src/main/java/com/iemr/admin/utils/JwtUserIdValidationFilter.java (2)
40-46: LGTM: Enhanced logging for CORS debugging.The additional logging for origin, method, URI, and configured allowed origins will significantly help with troubleshooting CORS-related issues.
48-66: Verify strictness of OPTIONS origin requirement.The filter correctly validates origins for security, but requiring an Origin header for all OPTIONS requests (line 49-53) and responding with 403 Forbidden may be overly strict. Some legitimate tools and health checks might send OPTIONS requests without an Origin header.
Consider whether the requirement for Origin on OPTIONS requests aligns with your operational needs. You may want to allow OPTIONS requests without Origin headers to pass through (without CORS headers) rather than blocking them with 403. For example:
if ("OPTIONS".equalsIgnoreCase(method)) { if (origin == null) { - logger.warn("BLOCKED - OPTIONS request without Origin header | Method: {} | URI: {}", method, uri); - response.sendError(HttpServletResponse.SC_FORBIDDEN, "OPTIONS request requires Origin header"); - return; + logger.debug("OPTIONS request without Origin header - allowing without CORS headers | URI: {}", uri); + response.setStatus(HttpServletResponse.SC_OK); + return; }This approach allows non-browser clients (monitoring tools, health checks) to function while still enforcing origin validation when an Origin header is present.
| private boolean isOriginAllowed(String origin) { | ||
| if (origin == null || allowedOrigins == null || allowedOrigins.trim().isEmpty()) { | ||
| return false; | ||
| } | ||
|
|
||
| return Arrays.stream(allowedOrigins.split(",")) | ||
| .map(String::trim) | ||
| .anyMatch(pattern -> { | ||
| String regex = pattern | ||
| .replace(".", "\\.") | ||
| .replace("*", ".*"); | ||
| return origin.matches(regex); | ||
| }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Code duplication: isOriginAllowed method.
This method duplicates identical logic found in JwtUserIdValidationFilter (lines 172-188). Code duplication creates maintenance overhead and risks inconsistencies when one copy is updated but not the other.
Extract this method to a shared utility class that both HTTPRequestInterceptor and JwtUserIdValidationFilter can use:
// New file: src/main/java/com/iemr/admin/utils/CorsOriginValidator.java
package com.iemr.admin.utils;
import java.util.Arrays;
import org.springframework.stereotype.Component;
@Component
public class CorsOriginValidator {
public boolean isOriginAllowed(String origin, String allowedOrigins) {
if (origin == null || allowedOrigins == null || allowedOrigins.trim().isEmpty()) {
return false;
}
return Arrays.stream(allowedOrigins.split(","))
.map(String::trim)
.anyMatch(pattern -> {
String regex = pattern
.replace(".", "\\.")
.replace("*", ".*");
return origin.matches(regex);
});
}
}Then inject and use this utility in both classes.
🤖 Prompt for AI Agents
In src/main/java/com/iemr/admin/utils/http/HTTPRequestInterceptor.java around
lines 153 to 166 the isOriginAllowed logic duplicates code in
JwtUserIdValidationFilter (lines ~172-188); extract that logic into a shared
utility class (e.g.,
src/main/java/com/iemr/admin/utils/CorsOriginValidator.java) with a public
boolean isOriginAllowed(String origin, String allowedOrigins) method
implementing the same validation, then remove the duplicate method from both
HTTPRequestInterceptor and JwtUserIdValidationFilter and inject
CorsOriginValidator (via @Component/@Autowired or constructor injection) into
both classes and call the new utility method instead.
There was a problem hiding this comment.
tolerable due to ease of readability
There was a problem hiding this comment.
@5Amogh, understood! I've noted your preference. The duplicated method is indeed small and self-contained, so keeping it local to each class can make the code easier to follow without jumping between files.
✏️ Learnings added
Learnt from: 5Amogh
Repo: PSMRI/Admin-API PR: 110
File: src/main/java/com/iemr/admin/utils/http/HTTPRequestInterceptor.java:153-166
Timestamp: 2025-11-17T11:55:39.481Z
Learning: In the Admin-API Java codebase, small self-contained utility methods like `isOriginAllowed` can be tolerated as duplicates across classes (e.g., HTTPRequestInterceptor and JwtUserIdValidationFilter) if it improves local readability and avoids unnecessary abstraction.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| if (origin != null && isOriginAllowed(origin)) { | ||
| response.setHeader("Access-Control-Allow-Origin", origin); | ||
| response.setHeader("Access-Control-Allow-Methods", "GET, POST, PUT, DELETE, OPTIONS"); | ||
| response.setHeader("Access-Control-Allow-Headers", "Authorization, Content-Type, Accept, Jwttoken"); | ||
| response.setHeader("Access-Control-Allow-Origin", origin); // Never use wildcard | ||
| response.setHeader("Access-Control-Allow-Methods", "GET, POST, PUT, PATCH, DELETE, OPTIONS"); | ||
| response.setHeader("Access-Control-Allow-Headers", | ||
| "Authorization, Content-Type, Accept, Jwttoken, serverAuthorization, ServerAuthorization, serverauthorization, Serverauthorization"); | ||
| response.setHeader("Access-Control-Allow-Credentials", "true"); | ||
| response.setHeader("Access-Control-Max-Age", "3600"); | ||
| logger.info("Origin Validated | Origin: {} | Method: {} | URI: {}", origin, method, uri); | ||
| } else { | ||
| logger.warn("Origin [{}] is NOT allowed. CORS headers NOT added.", origin); | ||
| } | ||
|
|
||
| if ("OPTIONS".equalsIgnoreCase(request.getMethod())) { | ||
| logger.info("OPTIONS request - skipping JWT validation"); | ||
| if ("OPTIONS".equalsIgnoreCase(method)) { | ||
| // OPTIONS (preflight) - respond with full allowed methods | ||
| response.setHeader("Access-Control-Allow-Origin", origin); | ||
| response.setHeader("Access-Control-Allow-Methods", "GET, POST, PUT, PATCH, DELETE, OPTIONS"); | ||
| response.setHeader("Access-Control-Allow-Headers", | ||
| "Authorization, Content-Type, Accept, Jwttoken, serverAuthorization, ServerAuthorization, serverauthorization, Serverauthorization"); | ||
| response.setHeader("Access-Control-Allow-Credentials", "true"); | ||
| response.setStatus(HttpServletResponse.SC_OK); | ||
| return; |
There was a problem hiding this comment.
Critical: CORS headers set unconditionally for OPTIONS requests.
There is a critical bug where CORS headers are set twice for OPTIONS requests, and more importantly, lines 84-92 execute even when the origin is NOT allowed. This means line 86 will set Access-Control-Allow-Origin to a potentially null or unauthorized origin, bypassing the security checks performed in lines 48-66.
The logic should be refactored to avoid duplication and ensure headers are only set for allowed origins:
- if (origin != null && isOriginAllowed(origin)) {
- response.setHeader("Access-Control-Allow-Origin", origin); // Never use wildcard
- response.setHeader("Access-Control-Allow-Methods", "GET, POST, PUT, PATCH, DELETE, OPTIONS");
- response.setHeader("Access-Control-Allow-Headers",
- "Authorization, Content-Type, Accept, Jwttoken, serverAuthorization, ServerAuthorization, serverauthorization, Serverauthorization");
- response.setHeader("Access-Control-Allow-Credentials", "true");
- response.setHeader("Access-Control-Max-Age", "3600");
- logger.info("Origin Validated | Origin: {} | Method: {} | URI: {}", origin, method, uri);
- } else {
- logger.warn("Origin [{}] is NOT allowed. CORS headers NOT added.", origin);
- }
-
if ("OPTIONS".equalsIgnoreCase(method)) {
- // OPTIONS (preflight) - respond with full allowed methods
- response.setHeader("Access-Control-Allow-Origin", origin);
- response.setHeader("Access-Control-Allow-Methods", "GET, POST, PUT, PATCH, DELETE, OPTIONS");
- response.setHeader("Access-Control-Allow-Headers",
- "Authorization, Content-Type, Accept, Jwttoken, serverAuthorization, ServerAuthorization, serverauthorization, Serverauthorization");
- response.setHeader("Access-Control-Allow-Credentials", "true");
+ // OPTIONS (preflight) - headers already set above if origin allowed
+ if (origin != null && isOriginAllowed(origin)) {
+ response.setHeader("Access-Control-Allow-Origin", origin);
+ response.setHeader("Access-Control-Allow-Methods", "GET, POST, PUT, PATCH, DELETE, OPTIONS");
+ response.setHeader("Access-Control-Allow-Headers",
+ "Authorization, Content-Type, Accept, Jwttoken, serverAuthorization, ServerAuthorization, serverauthorization, Serverauthorization");
+ response.setHeader("Access-Control-Allow-Credentials", "true");
+ response.setHeader("Access-Control-Max-Age", "3600");
+ logger.info("Origin Validated | Origin: {} | Method: OPTIONS | URI: {}", origin, uri);
+ }
response.setStatus(HttpServletResponse.SC_OK);
return;
+ } else if (origin != null && isOriginAllowed(origin)) {
+ // Non-OPTIONS requests with allowed origin
+ response.setHeader("Access-Control-Allow-Origin", origin);
+ response.setHeader("Access-Control-Allow-Methods", "GET, POST, PUT, PATCH, DELETE, OPTIONS");
+ response.setHeader("Access-Control-Allow-Headers",
+ "Authorization, Content-Type, Accept, Jwttoken, serverAuthorization, ServerAuthorization, serverauthorization, Serverauthorization");
+ response.setHeader("Access-Control-Allow-Credentials", "true");
+ response.setHeader("Access-Control-Max-Age", "3600");
+ logger.info("Origin Validated | Origin: {} | Method: {} | URI: {}", origin, method, uri);
}🤖 Prompt for AI Agents
In src/main/java/com/iemr/admin/utils/JwtUserIdValidationFilter.java around
lines 72-92, the OPTIONS branch unconditionally sets CORS headers (duplicating
the header code) even when the origin is not allowed, which can expose a
null/unauthorized origin; refactor so CORS headers are only set when origin !=
null and isOriginAllowed(origin): extract the repeated header-setting logic into
a single helper method (e.g., addCorsHeaders(response, origin)) and call it from
the origin-allowed branch and from the OPTIONS handling only after verifying
isOriginAllowed(origin) (or by moving the OPTIONS handling inside the
origin-allowed block); ensure Access-Control-Allow-Origin is never set to a
null/unauthorized value and remove the duplicated header lines.
There was a problem hiding this comment.
this has been fixed in the latest commit
There was a problem hiding this comment.
@5Amogh, thank you for the quick fix! I've verified the latest commit and can confirm the critical CORS security issue has been resolved. Great work! 🎉
The changes successfully address the problem by:
- ✅ Extracted CORS header logic into the
addCorsHeaders()helper method (lines 169-177), eliminating duplication - ✅ Properly gated CORS headers - they're now only set when
origin != null && isOriginAllowed(origin)(line 72) - ✅ Secured OPTIONS handling - moved inside the origin validation block, so unauthorized origins receive a 403 error instead of CORS headers
- ✅ Eliminated the security vulnerability - Access-Control-Allow-Origin is never set for null or unauthorized origins
The refactored code is much cleaner and maintains proper separation between authorized and unauthorized request handling.
/)/)
(^.^)
(> 🎉 Great fix!
🧠 Learnings used
Learnt from: 5Amogh
Repo: PSMRI/Admin-API PR: 110
File: src/main/java/com/iemr/admin/utils/http/HTTPRequestInterceptor.java:153-166
Timestamp: 2025-11-17T11:55:39.481Z
Learning: In the Admin-API Java codebase, small self-contained utility methods like `isOriginAllowed` can be tolerated as duplicates across classes (e.g., HTTPRequestInterceptor and JwtUserIdValidationFilter) if it improves local readability and avoids unnecessary abstraction.
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
|
| logger.debug("Request URI: {}", uri); | ||
| logger.debug("Allowed Origins Configured: {}", allowedOrigins); | ||
|
|
||
| if ("OPTIONS".equalsIgnoreCase(method)) { |
There was a problem hiding this comment.
@5Amogh The current implementation returns 200 OK for OPTIONS requests, which is the correct behavior for CORS preflight requests. Could you please check it and revert back to the original implementation? As per my understanding, if we return 403 for invalid origins, the browser displays a confusing error message. When we return 200 OK without CORS headers, the browser shows a clear and correct message: ‘CORS policy: No ‘Access-Control-Allow-Origin’ header is present
vishwab1
left a comment
vishwab1
left a comment
There was a problem hiding this comment.
need to change on OPTIONS logic
vishwab1
left a comment
vishwab1
left a comment
There was a problem hiding this comment.
need to change OPTIONS logic
| } else { | ||
| logger.warn("Origin [{}] is NOT allowed. CORS headers NOT added.", origin); | ||
|
|
||
| if ("OPTIONS".equalsIgnoreCase(method)) { |
📋 Description
JIRA ID: amm-1927
🎯 Summary
This PR transforms the CORS implementation from an overly restrictive, maintenance-heavy configuration to a secure, environment-aware, and developer-friendly approach. By removing endpoint-specific restrictions and fixing critical security vulnerabilities, we achieve:
Better Security: Environment-controlled origins, no hardcoded localhost, immutable configuration
Simpler Code: No endpoint-specific configuration, reduced maintenance
Proper Architecture: CORS handles browser security, endpoints handle authorization
Standard Compliance: Full REST API support including PATCH method
Result: A production-safe, maintainable CORS implementation that follows security best practices and reduces operational overhead.
✅ Type of Change
Summary by CodeRabbit
New Features
Improvements